Skip to content

docs: move SDK development guidance to local READMEs - #2253

Merged
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/contributing-updates
Aug 4, 2026
Merged

docs: move SDK development guidance to local READMEs#2253
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/contributing-updates

Conversation

@SteveSandersonMS

@SteveSandersonMS SteveSandersonMS commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep the top-level contribution guide focused on shared policy and links to each SDK README
  • add only the prerequisites and commands needed to install dependencies and run each SDK's tests
  • use the existing per-language manifests and READMEs as the source of version requirements

This is a smaller alternative to #2247 that avoids duplicating mutable language-specific instructions in CONTRIBUTING.md.

Validation

  • ran the documented Node.js workflow, including E2E tests
  • verified Python setup with uv sync and ran its formatting, linting, and type checks
  • checked every development command block for valid shell syntax and every top-level README link/anchor

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS
SteveSandersonMS requested a review from a team as a code owner August 4, 2026 15:08
Copilot AI balanced review requested due to automatic review settings August 4, 2026 15:08
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot mentioned this pull request Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves SDK-specific development guidance from the shared contribution guide into each SDK README.

Changes:

  • Adds prerequisites and validation workflows for all six SDKs.
  • Replaces duplicated contribution instructions with README links.
  • Documents pinned Rust and Java toolchains.
Show a summary per file
File Description
CONTRIBUTING.md Links to SDK-specific guidance.
nodejs/README.md Adds Node.js workflow.
python/README.md Adds Python workflow.
go/README.md Adds Go workflow.
dotnet/README.md Adds .NET workflow.
rust/README.md Adds Rust toolchain and workflow.
java/README.md Updates Java development workflow.

Review details

Suppressed comments (5)

python/README.md:1161

  • This local pytest command leaves GITHUB_ACTIONS unset. The E2E teardown therefore permits snapshot writes (python/e2e/conftest.py:40-42), and the shared proxy can contact live CAPI when a snapshot does not match. Document the replay-only invocation for routine contributor validation.
    nodejs/README.md:1126
  • This local test command leaves GITHUB_ACTIONS unset, so an E2E snapshot miss falls through to the live CAPI endpoint and successful runs can rewrite snapshot files (test/harness/replayingCapiProxy.ts:185-231,560-574). Make the documented contributor workflow replay-only to avoid network calls and fixture changes during routine validation.
    dotnet/README.md:1054
  • This local test invocation does not set GITHUB_ACTIONS; consequently E2ETestContext treats the run as record mode (dotnet/test/Harness/E2ETestContext.cs:476-478) and the replay proxy may contact live CAPI and write snapshots. The documented validation command should force replay-only mode.
    go/README.md:995
  • test.sh does not set GITHUB_ACTIONS, so this documented local workflow allows the shared replay proxy to call live CAPI on a snapshot miss and overwrite captures. Set replay-only mode explicitly for the contributor test run.
    rust/README.md:985
  • Plain cargo test does not exercise the Rust E2E suite because tests/e2e.rs is gated by test-support, and the two explicitly declared integration targets also require that feature (Cargo.toml:81-91). Enabling it also requires installing the CLI under nodejs/node_modules (tests/e2e/support.rs:1172-1203), which this workflow currently omits, so the documented test command gives a passing but incomplete validation. Install the Node runtime and run tests with test-support.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 4, 2026 15:13
Comment thread dotnet/README.md
Comment thread java/README.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (6)

rust/README.md:975

  • This command silently skips the Rust tests that are gated by test-support: session_test and protocol_version_test declare that required feature in Cargo.toml, while e2e.rs and jsonrpc_test.rs are entirely disabled without it. As written, contributors do not run the full SDK suite despite installing the E2E harness; enable test-support in the documented command.
cd rust && cargo test

dotnet/README.md:1043

  • The test workflow invokes npm ci, but Node.js is not listed as a development prerequisite. The harness enforces Node ^20.19.0 || >=22.12.0, so the current prerequisites are insufficient for running this command.
Development requires [.NET SDK 10+](https://dotnet.microsoft.com/download). From the repository root:

rust/README.md:971

  • The workflow below invokes npm ci, but this README does not list Node.js as a development prerequisite. Document the harness's Node ^20.19.0 || >=22.12.0 requirement so these instructions work in a fresh environment.
From the repository root:

java/README.md:425

  • mvn verify executes npm ci for both test/harness and nodejs through exec-maven-plugin, but Node.js is omitted from this prerequisite list. Add the harness's Node ^20.19.0 || >=22.12.0 requirement; otherwise the documented command fails in a JDK/Maven-only environment.
Development requires JDK 25+ and [Maven](https://maven.apache.org/download.cgi). From the repository root:

python/README.md:1148

  • The documented workflow invokes npm ci, but Node.js is not listed as a development prerequisite anywhere in this README. The harness requires Node ^20.19.0 || >=22.12.0, so contributors with no Node installation—or an older one—cannot follow these instructions.
Install [uv](https://docs.astral.sh/uv/), then from the repository root:

go/README.md:983

  • The test workflow below requires npm, but this README only lists Go as the relevant development tool. Document the test harness's Node ^20.19.0 || >=22.12.0 requirement so a fresh contributor can run the command successfully.
From the repository root:
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 15:18
Comment thread dotnet/README.md Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS

Copy link
Copy Markdown
Contributor Author

Addressed the valid review feedback by linking Python, Go, .NET, Rust, and Java development setup to the authoritative Node.js prerequisite. The Rust test-support command was already corrected in the preceding review-fix commit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS
SteveSandersonMS merged commit 69f2a46 into main Aug 4, 2026
27 checks passed
@SteveSandersonMS
SteveSandersonMS deleted the stevesa/contributing-updates branch August 4, 2026 15:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

CONTRIBUTING.md:38

  • The linked development sections generally document setup and tests, not standalone build commands, and deleting the former “All SDKs” note also removes the only explicit Node.js prerequisite even though the Python, Go, .NET, and Rust workflows below all invoke npm. Keep Node.js as a shared prerequisite here (linked to its maintained version source) and describe the delegated scope accurately.
Setup, build, and test instructions are maintained with each SDK:
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 4, 2026 15:24
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR modifies only documentation files (README.md files for all six SDKs and CONTRIBUTING.md). No SDK source code was changed.

The documentation changes are applied consistently across all six SDK implementations:

  • Node.js, Python, Go, .NET, Rust each receive a new ## Development section
  • Java's existing ### Development Setup section gets an updated prerequisite note
  • CONTRIBUTING.md is updated to link to per-SDK development instructions

No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for #2253 · sonnet46 9.19 AIC · ⌖ 5.33 AIC · ⊞ 6.6K ·

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

java/README.md:425

  • The prerequisite list omits tools required by the workflow immediately below: the commands invoke mvn, and the final validation phase requires a JDK 17 installation in addition to JDK 25. A contributor with only the listed prerequisites cannot complete the documented steps; list Maven and JDK 17 (or switch the commands to the checked-in Maven wrapper and make the JDK 17 phase explicitly optional).
Requires JDK 25 or later and a supported [Node.js version](../nodejs/README.md#prerequisites) for development. The following steps validate the artifact built with JDK 25 runs on both 25 and 17, preserving the MR-JAR behavior.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

edburns pushed a commit that referenced this pull request Aug 4, 2026
* docs: move SDK development guidance to local READMEs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs: simplify SDK development instructions

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs: address SDK development review feedback

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs: document Node requirement for SDK tests

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs: keep local test commands cross-platform

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants